Skip to content

Fix faulty edge-case logic in NParallelTextCorpus#295

Merged
johnml1135 merged 1 commit intomasterfrom
nparallel_corpus_successive_range_bug
Mar 17, 2025
Merged

Fix faulty edge-case logic in NParallelTextCorpus#295
johnml1135 merged 1 commit intomasterfrom
nparallel_corpus_successive_range_bug

Conversation

@Enkidu93
Copy link
Copy Markdown
Collaborator

@Enkidu93 Enkidu93 commented Mar 17, 2025 •
edited by ddaspit
Loading

Fixes #294

This was a bug that already existed in the previous ParallelTextCorpus implementation. We were relying on segment count as an indicator of a new range starting when we should just have used the IsRangeStart property since this isn't reliable when your target is totally empty.

This change is Reviewable

@Enkidu93 Enkidu93 requested review from ddaspit and johnml1135 March 17, 2025 16:31
@johnml1135
Copy link
Copy Markdown
Collaborator

johnml1135 commented Mar 17, 2025

tests/SIL.Machine.Tests/Corpora/ParallelTextCorpusTests.cs line 604 at r1 (raw file):

    [Test]
    public void GetRows_AdjacentRangesBothTexts_EmptyTargetSegments()
    {

I'm going to have to assume that this test catches the condition and would fail without the code changes. Is that correct and has that been verified?

johnml1135
johnml1135 reviewed Mar 17, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@johnml1135 johnml1135 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ddaspit and @Enkidu93)

@Enkidu93
Copy link
Copy Markdown
Collaborator Author

Enkidu93 commented Mar 17, 2025

tests/SIL.Machine.Tests/Corpora/ParallelTextCorpusTests.cs line 604 at r1 (raw file):

    [Test]
    public void GetRows_AdjacentRangesBothTexts_EmptyTargetSegments()
    {

I'm going to have to assume that this test catches the condition and would fail without the code changes. Is that correct and has that been verified?

Yes. I just re-verified. It exhibits the behavior Peter described when the code is reverted causing the test to fail.

johnml1135
johnml1135 approved these changes Mar 17, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@johnml1135 johnml1135 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ddaspit)

@johnml1135 johnml1135 merged commit c5fa125 into master Mar 17, 2025
4 checks passed
@johnml1135 johnml1135 deleted the nparallel_corpus_successive_range_bug branch March 17, 2025 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@johnml1135 johnml1135 johnml1135 approved these changes

@ddaspit ddaspit Awaiting requested review from ddaspit

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

NParallelTextCorpus does not properly handle successive ranges where the target has empty segments

2 participants

@Enkidu93 @johnml1135